-
Notifications
You must be signed in to change notification settings - Fork 0
added breakpoints logic #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| clearMediaQueryListenersForInteraction(interactionId: string): void { | ||
| this._removeMediaQueryListener(interactionId); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably delete this one
| mql.removeEventListener('change', handler); | ||
| this.mediaQueryListeners.delete(interactionId); | ||
| } | ||
| this.clearMediaQueryListenersForInteraction(interactionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this.clearMediaQueryListenersForInteraction(interactionId); | |
| this._removeMediaQueryListener(interactionId); |
packages/interact/src/core/add.ts
Outdated
| function _reconcile(root: IInteractElement, key: string): void { | ||
| remove(key); | ||
| add(root, key); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this function a static method of Interact, can be named update. This should solve the import problem
packages/interact/src/core/add.ts
Outdated
| id: string, | ||
| mql: MediaQueryList, | ||
| key: string, | ||
| handler: () => void, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this param with sourceRoot, and the handler can be hard-coded here as interact.update(sourceRoot, sourceKey)
packages/interact/src/core/add.ts
Outdated
| return; | ||
| } | ||
|
|
||
| mql.addEventListener('change', handler); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mql.addEventListener('change', handler); | |
| mql.addEventListener('change', () => Interact.update(sourceRoot, sourceKey)); |
packages/interact/src/core/add.ts
Outdated
| add(root, key); | ||
| } | ||
|
|
||
| function _setupMediaQueryListener( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all the logic here is tied to the instance, I suggest moving this function to be a method of the Interact instance itself. This should simplify everything, I think
| let matchMediaMock: ReturnType<typeof vi.fn>; | ||
|
|
||
| // Helper to create mock MediaQueryList objects | ||
| const createMockMQL = (query: string, initialMatches = false) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mock already exists at the top.
Also, since we're running in JSDOM env we may be able to remove some of the mocks
| matchMediaMock = vi.fn().mockImplementation((query: string) => createMockMQL(query)); | ||
|
|
||
| Object.defineProperty(window, 'matchMedia', { | ||
| writable: true, | ||
| configurable: true, | ||
| value: matchMediaMock, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already has a mock at the top, so first thing would be to reuse that.
| }; | ||
|
|
||
| beforeEach(() => { | ||
| Interact.destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be necessary
| Interact.destroy(); |
| const interactionId = getInterpolatedKey(interactionId_, key); | ||
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
| delete this.addedInteractions[interactionId]; | ||
| this._removeMediaQueryListener(interactionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary, since you're also calling clearMediaQueryListenersForKey right after this method.
Moreover, some of your interactionIds are completely made up and aren't stored in this Set, so they won't be reached in this branch.
| private _removeMediaQueryListener(id: string): void { | ||
| const listener = this.mediaQueryListeners.get(id); | ||
| if (listener) { | ||
| listener.mql.removeEventListener('change', listener.handler); | ||
| this.mediaQueryListeners.delete(id); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is redundant
| private _removeMediaQueryListener(id: string): void { | |
| const listener = this.mediaQueryListeners.get(id); | |
| if (listener) { | |
| listener.mql.removeEventListener('change', listener.handler); | |
| this.mediaQueryListeners.delete(id); | |
| } | |
| } |
| clearMediaQueryListenersForKey(key: string): void { | ||
| for (const [id, listener] of this.mediaQueryListeners.entries()) { | ||
| if (listener.key === key) { | ||
| this._removeMediaQueryListener(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably enough:
| this._removeMediaQueryListener(id); | |
| listener.mql.removeEventListener('change', listener.handler); | |
| this.mediaQueryListeners.delete(id); |
No description provided.